Introduce MaybeCompareFn trait to allow for partial compare specializations by gatesn · Pull Request #768 · vortex-data/vortex

gatesn · 2024-09-10T08:14:38Z

Currently an array needs to statically decide if it supports a compute function, prior to seeing the RHS or other arguments.

This change allows the array's compute function to see the RHS, and then decide it doesn't support the operation, enabling easier fallback.

This is only implemented for compare.

It has also highlighted the need for storage DType information to be available to an ExtScalar. Either by adding it to ExtDType, or adding it directly into that scalar variant. @a10y keen for your thoughts

Notes by @AdamGS:
To follow up @gatesn's work here, I added some other things to this PR:

I cleared up the boolean compare implementation as they were basically the arrow-based fallback we already had
Added an ALP MaybeCompareFn implementation that shows how the new interface looks

gatesn · 2024-09-10T08:15:51Z

-            };
-
-            Ok(Array::from_arrow(&boolean_array, true))
+            None


We could centralize the Arrow Array<->Scalar compute here since the entry function will swap the arguments for us.

AdamGS

LGTM

AdamGS · 2024-09-10T09:55:59Z

If there are no objections, I can start building on top of this PR. I think the general approach is good and we can start focusing on combinations we can/want to specialize.

there's no need to calculate states here, we can just use the length of a filtered array

a10y · 2024-09-10T14:33:01Z

@gatesn IIRC we had talked a couple months back about adding the storage DType to ExtensionDType. It makes sense to me that an ExtensionDType is just a logical type with some associated metadata. This still preserves flexibility of the physical encoding. I'd be a fan of adding the storage type into the ExtensionDType.

gatesn · 2024-09-10T14:44:59Z

The problem is, that means we would have to use e.g. int64 as the storage type for all date times, regardless of the unit.

In our Python prototype with had an IntWidthArray that allowed you to "reinterpret cast" a narrower int type into a wider one, e.g. you could zero-copy from Arrow an int32 array but pretend that it's int64 in Vortex.

It would be kind of weird to canonicalize back to Arrow by somehow introspecting this though...

gatesn · 2024-09-10T14:46:29Z

Ah, I had already thought of this: https://github.com/spiraldb/vortex/blob/0c4d74eb7afa359775493dea8d8de77841e727f5/vortex-scalar/src/extension.rs#L9-L10

There's something appealing about not prescribing the storage type in the DType. If we add it into the ExtScalar (it is already part of the ExtArray), then we have enough information in the right places.

robert3005 · 2024-09-10T14:54:56Z

I thought the storage type of the extdtype was the canonicalized storage type, and not required storage type?

robert3005 · 2024-09-11T11:58:19Z

+    let constant = rhs.slice(0, 1)?.clone();
+    let arrow_rhs = constant.into_canonical()?.into_arrow()?;
+
+    match arrow_lhs.data_type() {


you could compare the View types here

Oh but this is us being converted into arrow, ignore me. We should have a test though that this still works once we change canonicalization

gatesn added 2 commits September 9, 2024 20:31

Maybe Compare

163586f

Maybe Compare

00557e9

gatesn commented Sep 10, 2024

View reviewed changes

gatesn requested a review from AdamGS September 10, 2024 08:17

AdamGS reviewed Sep 10, 2024

View reviewed changes

Comment thread encodings/byte-bool/src/compute/mod.rs Outdated

This comment was marked as duplicate.

Sign in to view

AdamGS reviewed Sep 10, 2024

View reviewed changes

Comment thread vortex-array/src/array/varbin/compute/compare.rs Outdated

AdamGS added 5 commits September 10, 2024 11:08

.

f6e0dac

.

f56a8d1

Fix StructArray::filter length calculation (#769)

ee54e75

there's no need to calculate states here, we can just use the length of a filtered array

.

bc23e3c

.

a316f5f

AdamGS self-assigned this Sep 10, 2024

AdamGS added 5 commits September 10, 2024 14:51

cleanup, get clippy to pass and some small refactoring

9407870

Merge branch 'develop' into ngates/compare-fn

c123823

Merge branch 'develop' into adamg/alp-specialized-compare

e7a8b0a

.

4356049

merge ALP work

4455aa4

AdamGS added 7 commits September 10, 2024 16:00

.

4104c64

.

92f7bab

Merge branch 'develop' into ngates/compare-fn

5254954

.

8e7a55c

.

19ff6cd

.

4685a38

.

20ce4d0

.

a1c4201

AdamGS changed the title ~~Experiment with MaybeCompareFn~~ Introduce MaybeCompareFn trait to allow for partial compare specializations Sep 11, 2024

AdamGS added 2 commits September 11, 2024 11:08

Merge branch 'develop' into ngates/compare-fn

bef71c0

fix tests

c39d650

AdamGS marked this pull request as ready for review September 11, 2024 10:36

AdamGS self-requested a review September 11, 2024 11:57

robert3005 reviewed Sep 11, 2024

View reviewed changes

Comment thread vortex-array/src/array/primitive/compute/compare.rs Outdated

.

b84a91f

robert3005 reviewed Sep 11, 2024

View reviewed changes

Comment thread encodings/alp/src/compute.rs Outdated

robert3005 reviewed Sep 11, 2024

View reviewed changes

Comment thread encodings/alp/src/compute.rs Outdated

robert3005 reviewed Sep 11, 2024

View reviewed changes

Comment thread encodings/alp/src/compute.rs

AdamGS added 3 commits September 11, 2024 13:28

.

c9fec38

fn and tests

3c580df

.

9b1cfe0

AdamGS force-pushed the ngates/compare-fn branch from 370dc52 to 9b1cfe0 Compare September 11, 2024 12:54

robert3005 reviewed Sep 11, 2024

View reviewed changes

Comment thread encodings/alp/src/compute.rs

Add test for alp-const-null compare

17d5dc9

AdamGS force-pushed the ngates/compare-fn branch from b0524de to 17d5dc9 Compare September 11, 2024 12:59

.

047af1b

robert3005 reviewed Sep 11, 2024

View reviewed changes

Comment thread vortex-array/src/array/varbin/compute/compare.rs Outdated

Use VarBinBuilder

12db90f

robert3005 approved these changes Sep 11, 2024

View reviewed changes

AdamGS enabled auto-merge (squash) September 11, 2024 13:28

AdamGS added 2 commits September 11, 2024 14:29

Add default

d6b63b5

.

fbe03b1

AdamGS merged commit d54d83c into develop Sep 11, 2024

AdamGS deleted the ngates/compare-fn branch September 11, 2024 13:40

AdamGS mentioned this pull request Sep 11, 2024

[WIP] Some more perf-work #790

Closed

Conversation

gatesn commented Sep 10, 2024 • edited by AdamGS Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gatesn Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as duplicate.

Uh oh!

AdamGS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AdamGS commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a10y commented Sep 10, 2024

Uh oh!

gatesn commented Sep 10, 2024

Uh oh!

gatesn commented Sep 10, 2024

Uh oh!

robert3005 commented Sep 10, 2024

Uh oh!

robert3005 Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

robert3005 Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gatesn commented Sep 10, 2024 •

edited by AdamGS

Loading

AdamGS commented Sep 10, 2024 •

edited

Loading